Skip to content

improve: dependent resource can be re-initialized #2026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 24, 2023
Merged

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Aug 23, 2023

see linked issue

@openshift-ci openshift-ci bot requested review from adam-sandor and metacosm August 23, 2023 11:54
@csviri csviri changed the title Dr re initialization improve: dependent resource can be re-initialized Aug 23, 2023
@csviri csviri marked this pull request as draft August 23, 2023 11:54
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2023
@csviri csviri linked an issue Aug 23, 2023 that may be closed by this pull request
@shawkins
Copy link
Collaborator

@csviri csviri marked this pull request as ready for review August 23, 2023 12:54
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2023
@openshift-ci openshift-ci bot requested a review from andreaTP August 23, 2023 12:54
@@ -365,7 +365,7 @@ public void setConfigurationService(ConfigurationService configurationService) {
super.setConfigurationService(configurationService);

cache.addIndexers(indexerBuffer);
indexerBuffer = null;
indexerBuffer = new HashMap<>(0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that initializing the buffer to a zero size is a good idea since it will immediately need to get resized the first time something is added to it. Can't we initialize it to some default, reasonable size if we want to avoid overprovisioning for nothing? I'm not sure what's the typical use pattern here, like how many indexers might we expect to have?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We mostly shouldn't worry about initializing to specific sizes. The value is used lazily - so using zero or one is effectively the same. If not specified it will default to 16, but you're only holding that memory ephemerally based upon the usage pattern here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess 0 leads to the internal table being initialized to size 1… which is reasonable if we expect a low number of indexer. No idea on the usage pattern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually there is index just for one case (at least this is what we see now) when there is many-to-one / many-to-many relationship between resource, see: https://javaoperatorsdk.io/docs/features#managing-relation-between-primary-and-secondary-resources. But yeah, maybe not necessary to optimizations like this, can be even confusing to read. will remove this number if not objections and have the default (as when initialized).

@csviri csviri requested review from metacosm and shawkins August 24, 2023 10:26
@csviri csviri merged commit a973711 into main Aug 24, 2023
@csviri csviri deleted the dr-re-initialization branch August 24, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repeated initialization of a standalone dependent resource fails
3 participants